-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to LibGit2 v0.26.0 #22614
Update to LibGit2 v0.26.0 #22614
Conversation
} | ||
|
||
-static int check_host_name(const char *name, const char *host) | ||
-{ | ||
- if (!strcasecmp(name, host)) | ||
- return 0; | ||
- if (!strcasecmp(name, host)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like GitHub diff can't handle diffing diffs. The original patch removed the check_host_name
function and used the mbedtls function for doing cert verification instead of rolling our own in libgit2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To elaborate on my question mark, it appears the changes in the patch change the indentation. Seems unnecessary and kind of bloats the patch. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, is it just that the diff diffing is weird?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is totally possible the indentation was changed from spaces into tabs. libgit2/libgit2#4173 made the indentation into tabs so when I revised this patch there are now differences in the whitespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Thanks.
base/libgit2/libgit2.jl
Outdated
ENV["SSL_CERT_FILE"] | ||
else | ||
# If we have a bundled ca cert file, point libgit2 at that so SSL connections work. | ||
abspath(ccall(:jl_get_julia_home, Any, ()),Base.DATAROOTDIR,"julia","cert.pem") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor style point] few more spaces after the commas here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I didn't modify the styling when I moved this.
@@ -78,29 +78,14 @@ $(LIBGIT2_SRC_PATH)/libgit2-agent-nonfatal.patch-applied: $(LIBGIT2_SRC_PATH)/so | |||
patch -p1 -f < $(SRCDIR)/patches/libgit2-agent-nonfatal.patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we didn't upstream this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'll double check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like we did https://github.com/libgit2/libgit2/blob/master/src/transports/ssh.c#L300-L301
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LIBGIT2_BRANCH=v0.25.1 | ||
LIBGIT2_SHA1=2fcb8705e584ca61f6c4657525c9d2713f6a39d2 | ||
LIBGIT2_BRANCH=v0.26.0 | ||
LIBGIT2_SHA1=15e119375018fba121cf58e02a9f17fe22df0df8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should delete the old checksum files, and add the new ones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't interacted with our checksum files before. What's the standard procedure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git rm -r deps/checksums/libgit2-2fcb8705e584ca61f6c4657525c9d2713f6a39d2.tar.gz
git add deps/checksums/libgit2-15e119375018fba121cf58e02a9f17fe22df0df8.tar.gz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting. I was thinking I would have to retrieve the checksum myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you locally build a new dependency version and the checksum files aren't already present, it'll create them the first time - we add them to git so they get checked for everyone else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// OPTIONAL because REQUIRED drops the certificate as soon as the check | ||
// is made, so we can never see the certificate and override it. | ||
mbedtls_ssl_conf_authmode(git__ssl_conf, MBEDTLS_SSL_VERIFY_OPTIONAL); | ||
+ //mbedtls_ssl_conf_authmode(git__ssl_conf, MBEDTLS_SSL_VERIFY_NONE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was me trying to get the cert tests to co-operate. I'll remove this yet.
test/libgit2.jl
Outdated
@@ -369,7 +369,7 @@ mktempdir() do dir | |||
error("unexpected") | |||
catch e | |||
@test typeof(e) == LibGit2.GitError | |||
@test startswith(sprint(show,e),"GitError(Code:ENOTFOUND, Class:OS, Failed to resolve path") | |||
@test startswith(sprint(show,e),"GitError(Code:ENOTFOUND, Class:OS, failed to resolve path") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this makes the windows CI sad, since it uses libgit2 from the nightly binary instead of compiling from source
for the sake of distro packagers who might not be able to upgrade libgit2 versions right away, how about making this conditional on libgit2 version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
One of my patches isn't applying on Travis. Attempting to reproduce locally. |
test/libgit2.jl
Outdated
@@ -1910,8 +1921,11 @@ mktempdir() do dir | |||
deserialize(f) | |||
end | |||
@test err.code == LibGit2.Error.ERROR | |||
@test err.msg == "Invalid Content-Type: text/plain" | |||
@test err.msg == "invalid Content-Type: text/plain" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be version dependent as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests only run on Linux so they should always be using the specified version. I could and the conditional if it is still preferable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless a distro packager uses a different version of libgit2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😱 fair enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll update this and #22614 (comment) to be case insensitive
I can't seem to reproduce the issue locally. I think I'll push these commits individually to isolate the issue. |
6700ba7
to
82a1229
Compare
Looks like it was an issue with the Travis CI cache |
Trying a new PR to see if I can work around the Travis CI caching issues I'm encountering. |
Should be ready for final review. I've commented on libgit2/libgit2#4173 with the new changes this PR includes in the patches. |
$(LIBGIT2_SRC_PATH)/libgit2-mbedtls-verify.patch-applied \ | ||
$(LIBGIT2_SRC_PATH)/libgit2-gitconfig-symlink.patch-applied \ | ||
$(LIBGIT2_SRC_PATH)/libgit2-free-config.patch-applied \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this file be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it should. Good catch
Updates the version of LibGit2 to version 0.26.0 which includes various fixes. The change caused a few changes to the state of the libgit2 patches:
libgit2-mbedtls.patch
: revised to be based off of (mbedTLS support libgit2/libgit2#4173)libgit2-mbedtls-writer-fix.patch
: deleted, now included inlibgit2-mbedtls.patch
libgit2-gitconfig-symlink.patch
: deleted, fix included in libgit2 v0.26.0 (libgit2/libgit2@86a8cd9)libgit2-free-config.patch
: deleted, fix included in libgit2 v0.26.0 (libgit2/libgit2@5552237)libgit2-remote-push-NULL.patch
: deleted, fix included in libgit2 v0.26.0 (libgit2/libgit2@90cdf44)libgit2-mbedtls-verify.patch
: updated to work with newlibgit2-mbedtls.patch
. I should try to get this included in mbedTLS support libgit2/libgit2#4173libgit2-mbedtls-hotfix.patch
: various fixes to mbedTLS support libgit2/libgit2#4173Additionally the new
libgit2-mbedtls.patch
addresses libgit2/libgit2#3935 (comment) and should also fix #19135 (comment) and #20439TODOs: